Restructure metadata encoder to track deps precisely #35684

nikomatsakis · 2016-08-15T15:12:52Z

This issue restructures meta-data encoding to track dependencies very precisely. It uses a cute technique I hope to spread elsewhere, where we can guard the data flowing into a new dep-graph task and ensure that it is not "leaking" information from the outside, which would result in missing edges. There are no tests because we don't know of any bugs in the old system, but it's clear that there was leaked data.

The commit series is standalone, but the refactorings are kind of "windy". It's a good idea to read the comment in src/librustc_metadata/index_builder.rs to get a feeling for the overall strategy I was aiming at.

In several cases, I wound up adding some extra hashtable lookups, I think primarily for looking up AdtDef instances. We could remove these by implementing DepGraphRead for an AdtDef and then having it register a read to the adt-defs table, I guess, but I doubt it is really noticeable.

Eventually I think I'd like to extend this pattern to the dep-graph more generally, since it effectively guarantees that data cannot leak.

Fixes #35111.

r? @michaelwoerister

michaelwoerister · 2016-08-15T17:55:04Z

src/librustc_metadata/index_builder.rs

+//!
+//! In addition to the offset, we need to track the data that was used
+//! to generate the contents of each `data_item`. This is so that we
+//! can figure out which HIR nodes contributors to that data for


'contributed'

michaelwoerister · 2016-08-15T18:11:24Z

I like it! I'm definitely in favor of spreading the use of this pattern, it should indeed reduce the number of data leaks a lot.

michaelwoerister · 2016-08-15T18:13:33Z

src/librustc_metadata/index_builder.rs

+    fn read(&self, tcx: TyCtxt);
+}
+
+impl DepGraphRead for usize {


Why implement this? It seems open the door missing edges.

@michaelwoerister

Why implement this? It seems open the door missing edges.

A good question. I wanted it for indices that we pass down (e.g., the index of a variant), but it'd be semantically better to have some newtyped usize I suppose.

You could always pass such instances as part of a FromId wrapper, right?

nikomatsakis · 2016-08-16T15:27:55Z

@michaelwoerister and I chatted a bit on IRC and decided to remove the impl for usize and add an Untracked<T> wrapper instead, so that we can make local assertions that some particular usize is safe.

michaelwoerister · 2016-08-16T16:02:56Z

src/librustc_metadata/encoder.rs

        }
    }
 }

 impl<'a, 'tcx, 'encoder> ItemContentBuilder<'a, 'tcx, 'encoder> {
+    /// Encode data for the given variant of the given ADT. The
+    /// indices of the variant is untracked: this is ok because we


indices => index

michaelwoerister · 2016-08-16T16:06:50Z

r=me with the typos fixed.

nikomatsakis · 2016-08-16T16:20:44Z

@bors r=mw

bors · 2016-08-16T16:20:47Z

📌 Commit f0e5161 has been approved by mw

nikomatsakis · 2016-08-17T00:39:17Z

@bors r=mw

bors · 2016-08-17T00:39:18Z

📌 Commit 2f78c91 has been approved by mw

jonas-schievink · 2016-08-17T09:06:57Z

src/librustc_metadata/encoder.rs:1416:13: 1416:44 error: unresolved name `encode_bounds_and_type_for_item` [E0425]

src/librustc_metadata/encoder.rs:1416             encode_bounds_and_type_for_item(rbml_w, self.ecx, self.index, ty.id);

                                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/librustc_metadata/encoder.rs:1416:13: 1416:44 help: run `rustc --explain E0425` to see a detailed explanation

src/librustc_metadata/encoder.rs:1407:9: 1407:28 error: the trait bound `index_builder::ItemContentBuilder<'a, 'tcx, 'encoder>: rustc::hir::intravisit::Visitor<'_>` is not satisfied [E0277]

src/librustc_metadata/encoder.rs:1407         intravisit::walk_ty(self, ty);

                                              ^~~~~~~~~~~~~~~~~~~

src/librustc_metadata/encoder.rs:1407:9: 1407:28 help: run `rustc --explain E0277` to see a detailed explanation

src/librustc_metadata/encoder.rs:1407:9: 1407:28 note: required by `rustc::hir::intravisit::walk_ty`

src/librustc_metadata/encoder.rs:1410:32: 1410:58 error: attempted access of field `rbml_w_for_visit_item` on type `&mut index_builder::ItemContentBuilder<'a, 'tcx, 'encoder>`, but no field with that name was found

src/librustc_metadata/encoder.rs:1410             let rbml_w = &mut *self.rbml_w_for_visit_item;

                                                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~

src/librustc_metadata/encoder.rs:1412:25: 1412:35 error: attempted access of field `index` on type `&mut index_builder::ItemContentBuilder<'a, 'tcx, 'encoder>`, but no field with that name was found

src/librustc_metadata/encoder.rs:1412             let _task = self.index.record(def_id, rbml_w);

                                                              ^~~~~~~~~~

src/librustc_metadata/encoder.rs:1416:63: 1416:73 error: attempted access of field `index` on type `&mut index_builder::ItemContentBuilder<'a, 'tcx, 'encoder>`, but no field with that name was found

src/librustc_metadata/encoder.rs:1416             encode_bounds_and_type_for_item(rbml_w, self.ecx, self.index, ty.id);

                                                                                                    ^~~~~~~~~~

error: aborting due to 4 previous errors

bors · 2016-08-17T09:07:04Z

☔ The latest upstream changes (presumably #35605) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis · 2016-08-17T11:42:39Z

@jonas-schievink so weird; I do not see these locally (make check passes).

The idea is that ItemContentBuilder is a base-type of IndexBuilder.

The idea is that, this way, we can cleanly isolate ALL state that is being passed, since it goes as an argument to the fn pointer.

Also write a comment explaining the system.

The comment explains the `index-builder` pattern. We no longer need the `Deref/DerefMut` relationship, and it seems nicer without it.

The idea is that a `usize` is sort of ambiguous: in this case, it represents indices that do not need tracking, but it could as easily be some data read out from a tracked location, and hence represent tracked data. Therefore, we add an `Untracked` type that lets user assert that value is not tracked. Also correct various typos.

nikomatsakis · 2016-08-17T20:09:05Z

@bors r=mw

bors · 2016-08-17T20:09:06Z

📌 Commit 37d974f has been approved by mw

bors · 2016-08-17T21:48:11Z

⌛ Testing commit 37d974f with merge 895ed0e...

bors · 2016-08-17T23:38:43Z

💔 Test failed - auto-win-msvc-64-opt-rustbuild

nikomatsakis · 2016-08-18T14:23:14Z

@bors retry

nikomatsakis · 2016-08-18T14:23:19Z

/me hopes

bors · 2016-08-18T15:54:11Z

⌛ Testing commit 37d974f with merge 499484f...

@michaelwoerister

Restructure metadata encoder to track deps precisely This issue restructures meta-data encoding to track dependencies very precisely. It uses a cute technique I hope to spread elsewhere, where we can guard the data flowing into a new dep-graph task and ensure that it is not "leaking" information from the outside, which would result in missing edges. There are no tests because we don't know of any bugs in the old system, but it's clear that there was leaked data. The commit series is standalone, but the refactorings are kind of "windy". It's a good idea to read the comment in `src/librustc_metadata/index_builder.rs` to get a feeling for the overall strategy I was aiming at. In several cases, I wound up adding some extra hashtable lookups, I think primarily for looking up `AdtDef` instances. We could remove these by implementing `DepGraphRead` for an `AdtDef` and then having it register a read to the adt-defs table, I guess, but I doubt it is really noticeable. Eventually I think I'd like to extend this pattern to the dep-graph more generally, since it effectively guarantees that data cannot leak. Fixes #35111. r? @michaelwoerister

bors · 2016-08-18T21:02:25Z

rust-highfive assigned michaelwoerister Aug 15, 2016

nikomatsakis force-pushed the incr-comp-metadata-audit-35111 branch from 6f39c84 to 48a40dc Compare August 15, 2016 15:14

michaelwoerister reviewed Aug 15, 2016
View reviewed changes

nikomatsakis force-pushed the incr-comp-metadata-audit-35111 branch from d4d6380 to ef96c9d Compare August 16, 2016 15:51

michaelwoerister reviewed Aug 16, 2016
View reviewed changes

nikomatsakis force-pushed the incr-comp-metadata-audit-35111 branch from ef96c9d to f0e5161 Compare August 16, 2016 16:20

nikomatsakis force-pushed the incr-comp-metadata-audit-35111 branch from f0e5161 to 2f78c91 Compare August 17, 2016 00:39

move CrateIndex into its own module

b5fa8ab

nikomatsakis added 9 commits August 17, 2016 07:51

rename CrateIndex to IndexBuilder

85ac63e

store ecx, not dep-graph

25bb51d

put ecx into IndexBuilder so we don't have to pass

92f269e

move free encode fns into methods of IndexBuilder

b2c7922

introduce Deref/DerefMut to model subtype rel

6b76932

The idea is that ItemContentBuilder is a base-type of IndexBuilder.

make record take a closure

baccdc0

move rbml_w into the self struct

d49e1a9

separate main items from addl items in metadata

6277b1f

pull out the record call for encode_info_for_item

5166682

nikomatsakis added 12 commits August 17, 2016 10:38

pull out code for encoding enum variants

f351963

pull out encode_field

c716ad8

don't pass extra state fo encode_struct_ctor

9afcd77

pull out call to record for impl items

8dc8151

extract encode_info_for_trait_item into method

8c4a224

extract two more record calls

c0c8ab9

change callback for expr/type to a fn pointer

00e699f

The idea is that, this way, we can cleanly isolate ALL state that is being passed, since it goes as an argument to the fn pointer.

create a trait to ensure that data is tracked

f3990fe

Also write a comment explaining the system.

pacify the mercilous tidy

1a91e67

Add a comment, remove Deref/DerefMut

9daea5b

The comment explains the `index-builder` pattern. We no longer need the `Deref/DerefMut` relationship, and it seems nicer without it.

remove seemingly unnecessary call to encode_predicates

37d974f

nikomatsakis force-pushed the incr-comp-metadata-audit-35111 branch from 2f78c91 to 37d974f Compare August 17, 2016 19:26

bors merged commit 37d974f into rust-lang:master Aug 18, 2016

bors mentioned this pull request Aug 18, 2016

Remove the old AST-based backend from rustc_trans. #35764

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure metadata encoder to track deps precisely #35684

Restructure metadata encoder to track deps precisely #35684

nikomatsakis commented Aug 15, 2016

michaelwoerister Aug 15, 2016

michaelwoerister commented Aug 15, 2016

michaelwoerister Aug 15, 2016

nikomatsakis Aug 15, 2016

michaelwoerister Aug 15, 2016

nikomatsakis commented Aug 16, 2016

michaelwoerister Aug 16, 2016

michaelwoerister commented Aug 16, 2016

nikomatsakis commented Aug 16, 2016

bors commented Aug 16, 2016

nikomatsakis commented Aug 17, 2016

bors commented Aug 17, 2016

jonas-schievink commented Aug 17, 2016

bors commented Aug 17, 2016

nikomatsakis commented Aug 17, 2016

nikomatsakis commented Aug 17, 2016

bors commented Aug 17, 2016

bors commented Aug 17, 2016

bors commented Aug 17, 2016

nikomatsakis commented Aug 18, 2016

nikomatsakis commented Aug 18, 2016

bors commented Aug 18, 2016

bors commented Aug 18, 2016

Restructure metadata encoder to track deps precisely #35684

Restructure metadata encoder to track deps precisely #35684

Conversation

nikomatsakis commented Aug 15, 2016

michaelwoerister Aug 15, 2016

Choose a reason for hiding this comment

michaelwoerister commented Aug 15, 2016

michaelwoerister Aug 15, 2016

Choose a reason for hiding this comment

nikomatsakis Aug 15, 2016

Choose a reason for hiding this comment

michaelwoerister Aug 15, 2016

Choose a reason for hiding this comment

nikomatsakis commented Aug 16, 2016

michaelwoerister Aug 16, 2016

Choose a reason for hiding this comment

michaelwoerister commented Aug 16, 2016

nikomatsakis commented Aug 16, 2016

bors commented Aug 16, 2016

nikomatsakis commented Aug 17, 2016

bors commented Aug 17, 2016

jonas-schievink commented Aug 17, 2016

bors commented Aug 17, 2016

nikomatsakis commented Aug 17, 2016

nikomatsakis commented Aug 17, 2016

bors commented Aug 17, 2016

bors commented Aug 17, 2016

bors commented Aug 17, 2016

nikomatsakis commented Aug 18, 2016

nikomatsakis commented Aug 18, 2016

bors commented Aug 18, 2016

bors commented Aug 18, 2016